Fix robolectric test runtime failures due to x-large classpath #15487
Conversation
sgowroji
added
team-Android
| export JACOCO_IS_JAR_WRAPPED=1 | ||
| create_and_run_classpath_jar | ||
| java_ver=$(JAVABIN --version 2>&1) | head -1 | ||
| if [[ "$java_ver" =~ [^.0-9]1[.][1-8] ]]; then |
There was a problem hiding this comment.
This regex will accept versions 1.1, 1.2, 1.3, 1.4, 1.5, 1.6, 1.7, 1.8. Just want to double-check that this is the expected behavior. Do you care about version 1.0?
src/main/java/com/google/devtools/build/lib/bazel/rules/java/java_stub_template.txt
Show resolved
Hide resolved
oliviernotteghem
force-pushed
the
fix_largetest_classpath
branch
from
1230e43
to
5af4e3b
Compare
There was a problem hiding this comment.
I discussed this PR internally and we cannot accept these changes in their current state. Calling $JAVABIN just to get the version number takes around 100-200 ms; while this may not seem like a lot, if there are actions where this pattern occurs many times sequentially, that will quickly add up to be a major performance regression. Furthermore, we don't want to set the precedent of significantly bifurcating rule behavior based on the Java version, since it's a maintenance burden and in general not good for code health.
Instead of checking if the target JDK supports params based on the version number, it would be a better strategy to instead enforce this requirement in the platform runtime itself. This can be achieved by adding features = ["params_file"], to the various runtime declarations. There are a few places to make this modification:
- All of the java_repository-s in src/main/java/com/google/devtools/build/lib/bazel/rules/java/jdk.WORKSPACE.tmpl
- Modify remote_java_repository definition in rules_java/toolchains/remote_java_repository.bzl to accept features.
- Modify rules_java/toolchains/jdk.BUILD so that the java_runtime in there gets the features attribute.
Please let me know if you have any questions!
-Ted
ted-xie
added
awaiting-user-response
|
Hello @oliviernotteghem, Could you please respond to the above comment. Thanks! |
|
@sgowroji : I will take a look shortly and will get back to you before end of week. |
|
Hello @oliviernotteghem, Friendly Ping on the above request. Thanks! |
|
Hello @oliviernotteghem , This PR will be marked as stale and get closed in 7 days, Because it has no recent activity from many days. Could you please reply to the above comments to move this PR further state. Thank you. |
|
@ted-xie : Thanks for the input. However, I am not sure I will time to make your suggested changes without additional guidance. Can you explain the syntax and purpose of file remote_java_repository.bzl? Also, not sure what you mean by modifying jdk.BUILD to get the feature attribute. thanks. |
sgowroji
added
awaiting-review
fixes #14886
replaces #14887